Optional Python and R attributes within configuration file #2587
Conversation
…n sections and filling in of defaults when deploying. Also added new types and new unit tests.
| python: PythonExecutable | undefined, | ||
| r: RExecutable | undefined, |
There was a problem hiding this comment.
You could keep using ?, a bit simpler
python?: PythonExecutable,
r?: RExecutable,| python: python !== undefined ? python.pythonPath : undefined, | ||
| r: r !== undefined ? r.rPath : "", |
There was a problem hiding this comment.
... and here
python: python ? python.pythonPath : undefined,
r: r ? r.rPath : "",Same goes for all the other API methods below
| export type RExecutable = { | ||
| rPath: string; | ||
| }; | ||
| export function NewRExecutable(rPath: string): RExecutable { | ||
| return { | ||
| rPath, | ||
| }; | ||
| } | ||
|
|
||
| export type PythonExecutable = { | ||
| pythonPath: string; | ||
| }; | ||
| export function NewPythonExecutable(pythonPath: string): PythonExecutable { | ||
| return { | ||
| pythonPath, | ||
| }; | ||
| } |
There was a problem hiding this comment.
It is a bit strange having this types of constructors in TS which are more go style code.
It'd be good to be consistent and make a class for RExecutable and PythonExecutable, e.g
const exec = new RExecutable(path)| PackageFile string `toml:"package_file,omitempty" json:"packageFile"` | ||
| PackageManager string `toml:"package_manager,omitempty" json:"packageManager"` | ||
| } | ||
|
|
||
| func (p *Python) FillDefaults( | ||
| pythonInterpreter *interpreters.PythonInterpreter, |
There was a problem hiding this comment.
You shouldn't need to use pointers on interfaces,
removing * from here (pythonInterpreter interpreters.PythonInterpreter) will make things easier below:
pythonInterpreter.IsPythonExecutableValid()
| func (p *Python) FillDefaults( | ||
| pythonInterpreter *interpreters.PythonInterpreter, | ||
| ) { | ||
| if p != nil && pythonInterpreter != nil && (*pythonInterpreter).IsPythonExecutableValid() { |
There was a problem hiding this comment.
Since you are already within the struct method, I don't think you need to check p != nil
| python := *pythonInterpreter | ||
| if p.Version == "" { | ||
| pythonVersion, pythonVersionError := python.GetPythonVersion() |
There was a problem hiding this comment.
with the change suggested above, this will get easier: python := *pythonInterpreter
if p.Version == "" {
pythonVersion, pythonVersionError := pythonInterpreter.GetPythonVersion()
All these comments apply for R below too.
| rInterpreter *interpreters.RInterpreter | ||
| rMissingInterpreter *interpreters.RInterpreter | ||
| pythonInterpreter *interpreters.PythonInterpreter | ||
| pythonMissingInterpreter *interpreters.PythonInterpreter |
There was a problem hiding this comment.
Ditto here about interfaces and pointers, generally this isn't needed and complicates things.
Same applies for some other files with *interpreters.RInterpreter and *interpreters.PythonInterpreter.
…n sections and filling in of defaults when deploying. Also added new types and new unit tests.
Intent
The changes within this PR support the initialization of config files to have default attributes associated with R and Python dependencies, allowing the sections to be empty within the config file. When values are present, they are used by the back end, but when they are not present, the current values from the active interpreters are used, as the deployment manifest is built from the config file.
Resolves #2468
Resolves #2470
Type of Change
Approach
This is the second attempt to implement this functionality, with the previous one being closed without merging (#2568). I have simplified the approach within this PR and believe it is significantly addresses the concerns voiced during the previous PR's review.
The schema has been updated to allow all of the attributes present within the R and Python sections of the configuration to be optional. The presence of the section (
[r]or[python]) has been maintained as the indicator that the deployment has a dependency on the particular interpreter.Queries of a Configuration maintains its values as persisted. Empty attributes indicate that defaults will be applied.
The queries from the extension will return the configuration objects with empty attributes where defaults are expected. Because the extension relies upon having the attributes resolved (either from values present within the file or from the current default values determined by the active interpreter), the extension now calls a new API to get the default values from the active interpreters (GET /api/interpreters) and resolves the defaults within each configuration object prior to updating its state (store).
When a deployment is initiated, the agent resolves the defaults in the configuration file before generating the manifest file from it.
To remove any potential errors when passing in an R interpreter path vs. a Python interpreter path, I created a typescript type for each. This way I was able to get compilation errors rather than catching the coding issues during unit tests.
User Impact
New feature: The scenario goes like this:
Automated Tests
Unit tests were added and modified for the new functionality.
Directions for Reviewers
The important changes in order are:
The rest of the file updates are swaps to the new TS Types or signature changes.
Checklist